Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Metrics experiment #10826

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

Metrics experiment #10826

wants to merge 5 commits into from

Conversation

phryneas
Copy link
Member

@phryneas phryneas commented May 2, 2023

This is just an experiment about how a "metrics" usage story could look.

Setup:

import { MetricsLink } from "@apollo/client/link/metrics-link";
//...

const metricsLink = new MetricsLink();
const client = new ApolloClient({
  link: from([metricsLink, httpLink]),
  cache: new InMemoryCache(),
});
metricsLink.registerClient(client);
metricsLink.metrics.subscribe((metricEvent) => {
  console.log(metricEvent);
});

metricEvent here is one of

type CacheHit = {
  type: "cacheHit";
  operationType: "query" | "mutation" | "subscription";
  operationName: string;
  variables: unknown;
  traceId: string;
  startedAt: number;
};
type Request = {
  type: "request";
  operationType: "query" | "mutation" | "subscription";
  operationName: string;
  variables: unknown;
  traceId: string;
  startedAt: number;
  endedAt: number;
  responseCode: number;
  finishedAs: "success" | "error";
  errors?: readonly unknown[];
};

Checklist:

  • If this PR contains changes to the library itself (not necessary for e.g. docs updates), please include a changeset (see CONTRIBUTING.md)
  • If this PR is a new feature, please reference an issue where a consensus about the design was reached (not necessary for small changes)
  • Make sure all of the significant new logic is covered by tests

@changeset-bot
Copy link

changeset-bot bot commented May 2, 2023

🦋 Changeset detected

Latest commit: 53863d3

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@apollo/client Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@phryneas
Copy link
Member Author

phryneas commented May 2, 2023

/release:pr

@github-actions
Copy link
Contributor

github-actions bot commented May 2, 2023

A new release has been made for this PR. You can install it with npm i @apollo/[email protected].

@apollographql apollographql deleted a comment from github-actions bot May 2, 2023
@phryneas
Copy link
Member Author

phryneas commented May 3, 2023

Quick note: add a way to extract additional data, maybe with a extractInfo(context, operation?) method?

@phryneas
Copy link
Member Author

phryneas commented May 4, 2023

I added that extractInfo option, so it can now be used like this:

  const metricsLink = new MetricsLink({
    extractInfo(data) {
      if (data.type == "request") {
        return {
          url: data.context.response?.url,
        };
      }
      return {};
    },
  });
  const client = new ApolloClient({
    link: from([metricsLink, httpLink]),
    cache: new InMemoryCache(),
  });
  metricsLink.registerClient(client);
  metricsLink.metrics.subscribe((event) => {
    console.log(event);
  });

@phryneas
Copy link
Member Author

phryneas commented May 4, 2023

/release:pr

@github-actions
Copy link
Contributor

github-actions bot commented May 4, 2023

A new release has been made for this PR. You can install it with npm i @apollo/[email protected].

@netlify
Copy link

netlify bot commented May 4, 2023

Deploy Preview for apollo-client-docs ready!

Name Link
🔨 Latest commit f7192e5
🔍 Latest deploy log https://app.netlify.com/sites/apollo-client-docs/deploys/6453899c9e0bf50008f8a30b
😎 Deploy Preview https://deploy-preview-10826--apollo-client-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@netlify
Copy link

netlify bot commented May 4, 2023

Deploy Preview for apollo-client-docs ready!

Name Link
🔨 Latest commit 53863d3
🔍 Latest deploy log https://app.netlify.com/sites/apollo-client-docs/deploys/645389f28cfb54000892f2cb
😎 Deploy Preview https://deploy-preview-10826--apollo-client-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@Sumu-Ning
Copy link

There might be some cache metrics emissions missing for concurrent identical request.

Here is the procedures to reproduce:
I made 2 duplicate React Components, and each of them will trigger a call to a "slow" GraphQL service. Here are the cache metrics emissions:

 {"url":"\"http://localhost:8080/graphql\"","context":{},"type":"request","traceId":"1","operationType":"query","operationName":"SlowQuery","variables":{"milli":5000},"startedAt":1683693426023,"endedAt":1683693431042,"responseCode":200,"finishedAs":"success"}
 
 {"url":"\"http://localhost:8080/graphql\"","context":{},"type":"request","traceId":"3","operationType":"query","operationName":"SlowQuery","variables":{"milli":500},"startedAt":1683693432720,"endedAt":1683693433232,"responseCode":200,"finishedAs":"success"}

 {"url":"\"http://localhost:8080/graphql\"","context":{},"type":"request","traceId":"5","operationType":"query","operationName":"SlowQuery","variables":{"milli":50},"startedAt":1683693434674,"endedAt":1683693434733,"responseCode":200,"finishedAs":"success"}

 {"type":"cacheHit","traceId":"7","operationType":"query","operationName":"SlowQuery","variables":{"milli":500},"startedAt":1683693438818}
 
 {"type":"cacheHit","traceId":"8","operationType":"query","operationName":"SlowQuery","variables":{"milli":500},"startedAt":1683693438819}

If you check the traceId: for non-cached value, it shows 1, 3, 5, for cached value, it shows 7, 8.

I guess the good thing is: ApolloClient is optimized for concurrent identical calls, and the second request will not trigger network calls (Even if the cache is set to be network-only, it still behaves this way).

However, the latter of the concurrent requests does not emit any metrics.

Could you help investigate and fix this? Thank you!

@jerelmiller
Copy link
Member

My initial thoughts on this approach:

If we were strapped for time and absolutely had to ship something, I think this would be an acceptable approach. I think you've got a lot of really great things here as a start. I like the idea of your type and normalizing the response format as much as possible.

That being said, I'd love to see this feature go deeper and integrate throughout all of Apollo. Implementing this as a link can only get us so far toward this goal. The core, the cache, links, etc should all be able to report metrics to the this system. On top of that, it should be easy to attach to any given event.

We should also be thinking of this feature in terms of observability. Something that I think could make this feature super powerful is the ability to allow this to be part of a distributed trace. If we set this up correctly, users can trace performance all the way from Apollo Client to their backend systems, which would be incredibly powerful.

My North Star ™️ has always been Elixir's/Erlang's Telemetry library. I really like the way this is setup and integrates throughout the language. Not only that, but it has some great features:

  • Events are hierarchical
  • You can choose which events to attach to. You don't have to consume the firehose of every event
  • Using Telemetry.Metrics you can create metrics types for events (counter, summary, sum, etc.)

I'd also love to consider using something like OpenTelemetry which major vendors now support (New Relic, DataDog, etc). This makes it incredibly easy to hook up our telemetry system up to those backends. I imagine we can also use this in our own dev tools to provide some deep insights in development out of the box.

Hopefully this gives you some of the things I'd like to consider with this feature! Thanks for this experiment to give an idea of what could be possible.

@Sumu-Ning
Copy link

The new proposal from @jerelmiller will be more complete, however, I assume it will take longer time to implement too.
Will it be possible to release the current implementation, and deprecate it later when the new solution is released? (We hope to be able to collect such data sooner, and the Link implementation is unlikely to introduce any hard-to-fix tech debt)

Thank you!

@jerelmiller
Copy link
Member

@Sumu-Ning I'd prefer not releasing a premature version of this feature if we can help it. Releasing any version now means we will need to support it until we can make breaking changes in the next major version. We'd like to couple other major breaking changes in v4, and for that reason we are still a ways off from considering a major version bump.

If you're needing something now, here are some options I can recommend that lets you use this now without us releasing it in a public version. Be warned that these have some major downsides.

  1. We have the ability to generate snapshot releases, so we could create a snapshot release of this feature as it exists on this branch for you to use. Unfortunately its very difficult to make updates to snapshot releases, which means we need to keep this branch up-to-date in order for new snapshots to be generated with the latest version.
  2. You can create a fork of the repo that contains this feature and use your forked version until we get this done. This requires more manual integration on your part to pull in updated bugs/features as we release them.
  3. Create your own MetricsLink and use patch-package to update the Apollo internals to send this information. This feels the most robust of the options to me, but this also means you'll need to patch any new version that you update to.

These all suffer from the fact that getting updates is difficult, which means any bug fixes or new features that we introduce between now and when the metrics integration feature is done might be missed. In the case of patch-package, this also means creating patches for any version updates on your end. If you're considering one of these options, I'd prefer one that doesn't require us to keep a special branch up-to-date (option 1) and would prefer you pursue one of the other 2 options.

Just as a heads-up, given that this is likely not the final API, any integration with this feature as-is means a breaking change to you when this feature is released publicly. Depending on how tightly you integrate with it, this could be a painful change, especially if we make changes to the data shape of the events. If you're using this feature to send that data to a metrics backend of some kind (New Relic, DataDog, etc), these changes have the potential to destroy your measurements, unless you're careful to map the old data shape to the new one.

I totally get that you're excited about this feature and I wish I had a better answer for you. Since we serve so many developers, we need to be careful about the features we release and the decisions we make now to allow those features room to grow while allowing for backwards-compatible changes. I hope this helps rationalize why I'm hesitant to Just Release It, even though I know this would help a ton even in its current state. We want this feature to be awesome for everyone as I think this has a TON of potential to be impactful to a lot of developers that use Apollo.

@Sumu-Ning
Copy link

Thank you for your explanation @jerelmiller ! This makes a lot of sense from the software maintenance perspective, we totally understand.

At the same time, do you have any rough timeline when ApolloClient Major Version 4 will be released? This will be helpful for our planning too. Thank you!

@jerelmiller
Copy link
Member

@Sumu-Ning just to clarify, we should be able to land this feature in v3 in an upcoming minor release. We just won't get this into v3.8.0. @phryneas and I have a lot of interest in getting this feature landed so there is a good chance one of us will pick this up in the nearish future. I can't guarantee a timeline or version this will land in, only to say that you won't have to wait until v4 for this feature. We still need to work out what the final API will look like and what our initial scope of this feature will be.

The v4 comment I made was strictly to illustrate that if we release this feature as-is (done with an ApolloLink), we'd be stuck supporting this API until v4 where we could then introduce breaking changes. We are still planning what will make it into v4 and thus are still a ways off.

Hope that helps!

@Sumu-Ning
Copy link

Thank you very much for the update! We look forward to the release!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants